fix(aws-cdk): Improvements to IAM diff rendering #1542

rix0rrr · 2019-01-14T13:42:34Z

Contains the following changes:

Print table to STDOUT instead of STDERR. This ensures the diff output and the "confirm (y/n)" prompt that follows it don't interleave on the terminal, degrading readability.
Control table width to not exceed the terminal width, so that it doesn't wrap and lead to a mess of lines.
Switch back to the new version of the table library (instead of cli-table) which now has in-cell newline support, to get rid of the rendering bugs in cli-table that would occasionally eat newlines.
Render resource replace impact as red instead of yellow, to indicate that data loss will be happening here (fixes diff ui: replace is as dangerous as destroy - so should be in red #1458).
Change replacement diffing in order to not trigger IAM changes dialog (fixes IAM Diff Thrash with Lambda Permissions #1495).

Explanation:

We used to modify the new target template in place, causing changes to
the logical ids of replaced resources, which would trigger downstream
changes, which would further trigger potential downstream replacements, etc.

This would properly calculate the set of impacted resources, but would also lead to the modified logical ID popping up in the IAM diff, which was not desirable.

In this change, we do the same processing but we throw away the
template after all changes have been propagated, and only copy
the resultant property statuses onto the diff object that gets
returned to the user. This leads to the same displayed result
without the changes to the template actually propagating.

In the course of this modification, the diff classes have been changed
to also have objects in places of resources and properties that
didn't actually change (so that they could be modified in-place),
and diff objects have a boolean telling whether they are actual
changes or not.

Pull Request Checklist

Testing
- Unit test added
- CLI change?: manually run integration tests and paste output as a PR comment
- cdk-init template change?: coordinated update of integration tests with team
Docs
- jsdocs: All public APIs documented
- README: README and/or documentation topic updated
Title and Description
- Change type: title prefixed with fix, feat will appear in changelog
- Title: use lower-case and doesn't end with a period
- Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
- Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
Sensitive Modules (requires 2 PR approvers)
- IAM Policy Document (in @aws-cdk/aws-iam)
- EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
- Grant APIs (only if not based on official documentation with a reference)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

This ensure so that the diff output and the prompt don't interleave.

Calculcate column widths so that the table will not overflow the terminal anymore. Switch back to 'table' library which now has newline support and not the rendering bug that 'cli-table' had.

…roy'. Fixes #1458.

We used to modify the new template in place, causing changes to "logicalIDs" of replaced resources, which would trigger downstream changes and potential downstream replacements, etc. This would lead to the changed logical ID popping up in the IAM diff which was not desirable. In this change, we do the same processing but we throw away the template after all changes have been propagated, and only copy the resultant property STATUSES onto the diff object that gets returned to the user. This leads to the same displayed result without the changes to the template actually propagating. In the course of this modification, the diff classes have been changed to also have objects in places of resources and properties that didn't actually change (so that they could be modified in-place), and diff objects have a boolean telling whether they are actual changes or not. Fixes #1495.

eladb · 2019-01-14T14:10:59Z

Could you please upload a nice little screenshot?

rix0rrr · 2019-01-14T14:29:27Z

This PR is blocked on gajus/table#89

rix0rrr · 2019-01-14T14:40:31Z

sam-goodwin

Table looks beautiful! I had some comments but nothing worth blocking on.

sam-goodwin · 2019-01-16T04:29:42Z

packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts

+ count += this.outputs.differenceCount;
+ count += this.parameters.differenceCount;
+ count += this.resources.differenceCount;
+ count += this.unknown.differenceCount;


nit: scope these under a single object so you can enumerate and reduce?

sam-goodwin · 2019-01-16T04:37:49Z

packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts

+ [ResourceImpact.WILL_ORPHAN]: 3,
+ [ResourceImpact.MAY_REPLACE]: 4,
+ [ResourceImpact.WILL_REPLACE]: 5,
+ [ResourceImpact.WILL_DESTROY]: 6,


Why declare this in the function scope?

Why not? It's only used here.

Is it optimized away? :)

sam-goodwin · 2019-01-16T04:53:21Z

packages/@aws-cdk/cloudformation-diff/lib/format-table.ts

+ widths.forEach((width, i) => {
+ ret[i] = { width, useWordWrap: true } as any; // 'useWordWrap' is not in @types/table
+ if (width === undefined) {
+ delete ret[i].width;


This undefined terminal width has penetrated the logic everywhere. Can we not switch it on/off in a simpler way to avoid delete ret[i].width and Array<number | undefined>? It seems to have a single source: formatTable(_, columns: number | undefined).

sam-goodwin · 2019-01-16T04:53:57Z

packages/@aws-cdk/cloudformation-diff/lib/format-table.ts

+ const columns = rows[0].map((_, i) => Math.max(...rows.map(row => stringWidth(row[i]))));
+
+ // If we have no terminal width, do nothing
+ if (terminalWidth === undefined) { return columns.map(_ => undefined); }


array of undefined? Is this the optimal signal?

I did it this way because I wanted the useWordWrap: true for every column.

But I suppose that's silly, because if there's no width, what does it even matter how you wrap?

sam-goodwin · 2019-01-16T04:55:38Z

packages/@aws-cdk/cloudformation-diff/lib/format-table.ts

+ return ret;
+}
+
+function sum(xs: number[]): number {


xs.reduce((a,b) => a + b, 0)

Did it this way on purpose because I believe V8 JITs this better. But I will agree it's a micro-optimization :)

sam-goodwin · 2019-01-16T04:57:52Z

packages/aws-cdk/lib/util/tables.ts

- return table.toString();
+ if (options.colWidths) {
+ options.colWidths.forEach((width, i) => {
+ columns[i] = { width, useWordWrap: true } as any; // @types doesn't have this type


Why do you need to cast to any here? You have the type, table.ColumnConfig, is this not the correct shape?

As the comment says, The @types package for this library is for 4.x.x and we're using 5.x.x. There is no newer one and I don't really feel like writing it... :(

sam-goodwin · 2019-01-16T04:58:31Z

packages/aws-cdk/lib/util/tables.ts

+ joinLeft: tableColor('├'),
+ joinRight: tableColor('┤'),
+ joinJoin: tableColor('┼')
+};


I've seen this table twice .. redundant?

One is in @aws-cdk/cloudformation-diff and the other one is in aws-cdk. I'd prefer to keep the table rendering private to the package, because it doesn't seem like an exportable feature of @aws-cdk/cloudformationd-diff`, and extracting to another package just to share seemed a bit too much.

But, otoh, terminal width wrapping now only works for the IAM changes table, not for the others. Maybe I should share...

Rewrite 'undefined' logic to make it less onerous. Print a message instead of an empty table if 'cdk context' has no information in it (fixes #1549).

rix0rrr added 7 commits January 11, 2019 13:49

Print diff to stdout just like the confirmation prompt

07264c4

This ensure so that the diff output and the prompt don't interleave.

Fix table rendering

f0975c1

Calculcate column widths so that the table will not overflow the terminal anymore. Switch back to 'table' library which now has newline support and not the rendering bug that 'cli-table' had.

Format 'replace' as red as well since it's just as dangerous as 'dest…

e4fec19

…roy'. Fixes #1458.

Fix toolkit

ebb7f34

Fix toolkit integ 'ls' test

cf8abc3

Fix base case for 'resourceImpact'

a9d0a04

rix0rrr requested a review from a team as a code owner January 14, 2019 13:42

rix0rrr requested review from eladb and RomainMuller January 14, 2019 13:42

Fix test

7a78b5f

Switch version of 'table' that does not filter out newlines anymore

afe38df

sam-goodwin approved these changes Jan 16, 2019

View reviewed changes

Use same table formatting in CDK toolkit as CFN-diff

963920e

Rewrite 'undefined' logic to make it less onerous. Print a message instead of an empty table if 'cdk context' has no information in it (fixes #1549).

rix0rrr merged commit 3270b47 into master Jan 16, 2019

rix0rrr deleted the huijbers/iam-diff-improvements branch January 16, 2019 10:30

rix0rrr mentioned this pull request Jan 23, 2019

ux: IAM Statement Changes table is too big and unreadable sometimes #1600

Closed

NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(aws-cdk): Improvements to IAM diff rendering #1542

fix(aws-cdk): Improvements to IAM diff rendering #1542

rix0rrr commented Jan 14, 2019

eladb commented Jan 14, 2019

rix0rrr commented Jan 14, 2019

rix0rrr commented Jan 14, 2019

sam-goodwin left a comment

sam-goodwin Jan 16, 2019

sam-goodwin Jan 16, 2019

rix0rrr Jan 16, 2019

sam-goodwin Jan 16, 2019

sam-goodwin Jan 16, 2019

sam-goodwin Jan 16, 2019

rix0rrr Jan 16, 2019

sam-goodwin Jan 16, 2019

rix0rrr Jan 16, 2019

sam-goodwin Jan 16, 2019

rix0rrr Jan 16, 2019

sam-goodwin Jan 16, 2019

rix0rrr Jan 16, 2019

rix0rrr Jan 16, 2019

fix(aws-cdk): Improvements to IAM diff rendering #1542

fix(aws-cdk): Improvements to IAM diff rendering #1542

Conversation

rix0rrr commented Jan 14, 2019

Pull Request Checklist

eladb commented Jan 14, 2019

rix0rrr commented Jan 14, 2019

rix0rrr commented Jan 14, 2019

sam-goodwin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment